CpGrid: build point communication interfaces from all points of a cell - #1056
CpGrid: build point communication interfaces from all points of a cell#1056hnil wants to merge 5 commits into
Conversation
hnil
added
the
manual:bugfix
blattms
left a comment
blattms
left a comment
There was a problem hiding this comment.
I need to think about this a little. Did not look at the tests. So this is a first assessement. More to follow,
It is nice to have this fixed. We never needed communication between points before, hence this surely needed some love.
I think we should make sure that we do not add permanent addtional memory usage and maybe we make this more efficient, even.
| const std::size_t nc = cell_to_point_.size(); | ||
| std::vector<int> points; | ||
| for (std::size_t cell = 0; cell < nc; ++cell) { | ||
| points.clear(); | ||
| const auto& faces = cell_to_face_[cpgrid::EntityRep<0>(cell, true)]; | ||
| const int nf = faces.size(); | ||
| for (int f = 0; f < nf; ++f) { | ||
| for (const auto& fv : face_to_point_[faces[f].index()]) { | ||
| if (std::find(points.begin(), points.end(), fv) == points.end()) { | ||
| points.push_back(fv); | ||
| } | ||
| } | ||
| } | ||
| cell_to_allpoint_.appendRow(points.begin(), points.end()); | ||
| } |
There was a problem hiding this comment.
I think the best place to do this is during (computeFace2Point)[https://github.com/hnil/opm-grid/blob/99aa1cc1f1761123ba5d75e3f94d4cde8d108ea9/opm/grid/cpgrid/CpGridData.cpp#L1241] namely during the last grid.scatterData which uses a FaceViaCellHandleWrapper<SparseTableDataHandle>
One possibility would be to optionally make SparseTableDataHandle populate your container with the values during its scatter method.
| mark_.resize(size(0)); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
We should not add trailing whitespace
| /** @brief All points of each cell, i.e. every point of every face of the | ||
| * cell, not only the eight canonical corners. On corner-point grids | ||
| * with hanging nodes the extra points are essential for building | ||
| * complete codim-3 communication interfaces. */ | ||
| Opm::SparseTable<int> cell_to_allpoint_; |
There was a problem hiding this comment.
This is not really used after the set up and should be a local variable in some function
| { | ||
| using DataType = typename Handle::DataType; | ||
| using C2PTable = std::vector< std::array<int,8> >; | ||
| using C2PTable = Opm::SparseTable<int>; |
There was a problem hiding this comment.
I don't think we even use the PointViaCellHandle. So maybe remove it?
There was a problem hiding this comment.
Removed. Checked first that nothing uses it - not opm-grid, not opm-simulators, and not opm-flowgeomechanics - and PointViaCellWarner existed only as its base, so that and the translation unit holding the warning went too. 113 lines. All 84 opm-grid tests pass.
New 4-rank test (partition_of_unity_test): a 4x4x2 corner-point grid whose
right half is shifted down by half a cell, partitioned so the fault plane
coincides with a processor boundary. Fault-face processing produces hanging
nodes -- face nodes that are not canonical corners of the neighbouring cell.
The test communicates a codim-3 data handle over All_All_Interface and
requires every non-interior vertex to receive at least one message.
Before the all-points communication interface fix each rank reports two
unreached hanging nodes ('Owner is not a partition of unity' in vertex-based
mechanics assembly); with cell_to_allpoint_-based interfaces the test passes.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The codim-3 (point) communication interface and the point-via-cell data handle were built from cell_to_point_, i.e. only the eight canonical corners of each cell. On corner-point grids with hanging nodes a face can reference nodes that are not canonical corners of the neighbouring cell; such nodes never entered point_interfaces_, so they were invisible to codim-3 communication and their ownership failed to be a partition of unity across ranks (observed as 'Owner is not a partition of unity' in vertex-based mechanics assembly). Introduce cell_to_allpoint_ (SparseTable: every node of every face of each cell, sorted and deduplicated) and use it for the point interface and for PointViaCellHandleWrapper. AttributeDataHandle::fixedSize() now returns false since rows vary in length, and row iteration uses the SparseTable iterators. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lone - cell_to_allpoint is now a local in the function that builds the point interfaces, so nothing is retained after set-up. This was the main objection: no permanent additional memory. - The duplicate check is a per-cell marker array instead of std::find, so building the rows is linear rather than quadratic in the points of a cell. Traversal order is untouched, which is the invariant the positional gather/scatter depends on. - PointViaCellHandleWrapper is left exactly as it was; its C2PTable typedef no longer needs changing. It has no users at all, but removing it is a separate cleanup. - Dropped the trailing whitespace this branch introduced. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
99aa1cc to
a4c0e2e
Compare
|
Thanks — three of four done, and the memory objection is fully addressed.
Not done: building this during Rebased onto master (after #1051). np 1/2/4 pass, full opm-grid suite 84/84. |
The file was added with a SINTEF header copied from a neighbour; the work is Equinor's. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
What do I do with this @blattms |
PointViaCellHandleWrapper has no users anywhere - not in opm-grid, not in opm-simulators, not in opm-flowgeomechanics - and PointViaCellWarner existed only to serve it, so both go, along with the translation unit that held the warning. Removing dead code rather than carrying it through the point-communication rework this PR performs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 participants
On a faulted corner-point grid the hanging nodes along the fault are not canonical corners of the neighbouring cells, so point interfaces built from the 8 canonical corners never include them: they receive no message and vertex ownership is not a partition of unity across ranks (breaks vertex-based fields, e.g. mechanics).
First commit adds a test that fails on master (np 2/4); second builds the interfaces from all points of a cell, after which np 1/2/4 pass and
distribution_testis unchanged.One invariant worth stating: the per-cell point rows are traversal-order deduplicated, deliberately not sorted — the interface pairs sender/receiver rows positionally across ranks, and local point indices differ per rank, so sort+unique would silently reintroduce the bug.